replace org_user deactivation with removal of org_user role #4477

elasticspoon · 2024-06-23T20:27:42Z

Resolves #3587

Description

Problem

The issue was that banks could "deactivate" a user (which is essentially a soft delete of the whole User record). However, if that user then went to a Partner they would not be able to invite that user because that user would exist (new User cannot be created) but be soft deleted (existing User is treated like they didn't have permissions).

Solution

Roles got added in #3117. Therefore, the logic around deactivation should address the role not the user.

However, there is a caveat. I attempted to implement logic around deactivating roles themselves in #4392, #4382. But the logic became very complex. We decided (#3587 (comment)) to go for a more architerurally simple solution of removing the deactivation functionality and just deleting the roles.

Drawbacks

There are two main drawbacks with this approach.

Existing Data

What do we do with existing discarded users? Discarded == soft deleted. I can imagine a world where a user was discarded without that meaning to be a "deactivation".

My current idea is that any User that is discarded and only had the org_user role was probably a deactivation and we can reactivate them and remove the org_user role from them. But this requires some more information.

Loss of Granularity

Previously we had 3 states for org_users: active, not an org_user and deactivated (meaning they were a user at one point but not any more).

Now there are only 2 states: user or not. This means we have a bit less information, we can no longer track if someone was once a user but not anymore. Does this matter? Probably not.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Request + System specs

Screenshots

elasticspoon · 2024-06-24T18:01:44Z

config/initializers/devise.rb

+
+Warden::Manager.after_set_user do |user, auth, opts|
+  if user.roles.empty?
+    auth.logout
+    throw(:warden)
+  end
+end


This little change deals with the situation where a user has no roles (in that case we don't let them log in). This mirrors the behavior of dealing with discarded users (that is done with a default scope on users default_scope { kept }).

I considered 2 other options:

adding to the default scope to filter out users without roles but I was worried this could affect existing behavior

attempted to modify the create method of the session controller but that resulted in a confusing error for the user (it would seem like they logged in and got logged out immediately). I needed to address the logic in the super method.

elasticspoon · 2024-06-24T18:02:57Z

app/controllers/organizations_controller.rb

-
-  def reactivate_user
-    user = User.with_discarded.find_by!(id: params[:user_id])
+  def remove_user


I modified this method rather than using the admin roles controller because that you required the super admin role and I thought it did not makes sense to alter that logic

elasticspoon · 2024-06-24T18:04:57Z

spec/system/log_in_system_spec.rb

+      fill_in "user_email", with: "no_role_user@example.com"
+      fill_in "user_password", with: DEFAULT_USER_PASSWORD
+      find('input[name="commit"]').click
+      expect(page).to have_content("You need to sign in before continuing.")


I don't love this error message but I could not figure out how to change it.

I tried throw(:warden, action: :invalid) but that still gave the same error message.

Not ideal, but this kind of code is super hairy. I'd timebox it and if you can't change it, I'm OK with it as is. @cielf for final decision here.

The issue was that banks could "deactivate" a user (which is essentially a soft delete of the whole User record). However, if that user then went to a Partner they would not be able to invite that user because that user would exist (new User cannot be created) but be soft deleted (existing User is treated like they didn't have permissions). We changed the logic around deactivation to address the role not the user. I attempted to implement logic around deactivating roles themselves in rubyforgood#4392, rubyforgood#4382. But the logic became very complex. We decided (rubyforgood#3587 (comment)) to go for a more architerurally simple solution of removing the deactivation functionality and just deleting the roles. To migrate the existing data I decided to reactivate any discarded user with the org user role and remove the org user role from them. (My guess is that those are the org users that actually got deactivated)

elasticspoon · 2024-06-24T19:10:40Z

db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb

This is what I came up with in terms of a data migration. I am not totally sure it make sense. But my logic was: a discarded user with the 'org_user' role is probably someone that got deactivated.

dorner · 2024-06-26T19:23:17Z

db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb

+        user.roles.where(name: "org_user").delete_all
+      end
+    end
+  end


I checked the data, and every discarded user has an org_user role. This makes sense, because you can only discard a user from the organization user management page. So I think you're safe to not have to check the role name.

In fact, you should just delete all roles that belong to current discarded users - many of them also have partner roles, but if they're discarded, they wouldn't have been able to log in. So effectively all their roles should be removed.

Updated this. Since all discarded users are org users might as well simplify it further and just deal exclusively with discarded users.

spec/requests/organization_requests_spec.rb

spec/requests/partners/user_requests_spec.rb

dorner · 2024-06-26T19:25:32Z

spec/system/log_in_system_spec.rb

+      fill_in "user_email", with: "no_role_user@example.com"
+      fill_in "user_password", with: DEFAULT_USER_PASSWORD
+      find('input[name="commit"]').click
+      expect(page).to have_content("You need to sign in before continuing.")


Not ideal, but this kind of code is super hairy. I'd timebox it and if you can't change it, I'm OK with it as is. @cielf for final decision here.

also simplification/correction of some test logic

All discarded users are org_user meaning they have been deactivated. To simplify everything we will just remove all thier roles.

cielf · 2024-06-27T23:06:58Z

I hope it's going to be a pretty rare case that someone is going to try to sign in after their role has been deleted, so I'm ok with leaving the slightly confusing error message. Not the best place to be using our resources if it's too hairy.

dorner · 2024-07-03T15:46:25Z

Looks good to me! @cielf did you want to take a look?

cielf · 2024-07-03T16:15:44Z

Looks good to me! @cielf did you want to take a look?

Yes.

cielf

LGTM also

github-actions · 2024-07-07T14:52:54Z

@elasticspoon: Your PR replace org_user deactivation with removal of org_user role is part of today's Human Essentials production release: 2024.07.07.
Thank you very much for your contribution!

elasticspoon force-pushed the feat/replace-role-deactivation-with-removal branch 2 times, most recently from 23f1269 to 3a66ddc Compare June 24, 2024 00:36

elasticspoon changed the title ~~wip~~ replace org_user deactivation with removal of org_user role Jun 24, 2024

elasticspoon commented Jun 24, 2024

View reviewed changes

elasticspoon marked this pull request as ready for review June 24, 2024 18:05

elasticspoon requested a review from dorner June 24, 2024 18:05

elasticspoon force-pushed the feat/replace-role-deactivation-with-removal branch from 3a66ddc to e817a2a Compare June 24, 2024 19:04

elasticspoon commented Jun 24, 2024

View reviewed changes

dorner requested changes Jun 26, 2024

View reviewed changes

elasticspoon added 2 commits June 26, 2024 16:29

fix: review comments

d77b19c

also simplification/correction of some test logic

fix: simplify migration

63f2a9c

All discarded users are org_user meaning they have been deactivated. To simplify everything we will just remove all thier roles.

elasticspoon requested review from dorner and cielf June 26, 2024 21:05

dorner approved these changes Jul 3, 2024

View reviewed changes

cielf approved these changes Jul 3, 2024

View reviewed changes

cielf merged commit 841b0ed into rubyforgood:main Jul 3, 2024
19 checks passed

elasticspoon deleted the feat/replace-role-deactivation-with-removal branch July 3, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace org_user deactivation with removal of org_user role #4477

replace org_user deactivation with removal of org_user role #4477

elasticspoon commented Jun 23, 2024 •

edited

Loading

elasticspoon Jun 24, 2024

elasticspoon Jun 24, 2024

elasticspoon Jun 24, 2024

dorner Jun 26, 2024

elasticspoon Jun 24, 2024

dorner Jun 26, 2024

elasticspoon Jun 26, 2024

dorner Jun 26, 2024

cielf commented Jun 27, 2024

dorner commented Jul 3, 2024

cielf commented Jul 3, 2024

cielf left a comment

github-actions bot commented Jul 7, 2024

replace org_user deactivation with removal of org_user role #4477

replace org_user deactivation with removal of org_user role #4477

Conversation

elasticspoon commented Jun 23, 2024 • edited Loading

Description

Problem

Solution

Drawbacks

Existing Data

Loss of Granularity

Type of change

How Has This Been Tested?

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf commented Jun 27, 2024

dorner commented Jul 3, 2024

cielf commented Jul 3, 2024

cielf left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 7, 2024

elasticspoon commented Jun 23, 2024 •

edited

Loading